Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(nodes-base): fix and harmonize all primaryDocumentation links #4191

Merged
merged 6 commits into from
Sep 29, 2022

Conversation

mieky
Copy link
Member

@mieky mieky commented Sep 26, 2022

This is a housekeeping 🏠 🔨 PR to bring all the primaryDocumentation links in nodes-base up to date and help @StarfallProjects keep them tidy.

With the help of a few custom scripts, I was able to locate and fix a couple of 404 links, malformed JSONs, and unnecessary redirects. The bulk of this change is to crawl all primaryDocumentation links in node descriptor files, and for those that would be redirected (HTTP 301), replace the JSON with their actual target.

Finally, ran npx prettier --write nodes/**/*.node.json to pretty-print the files.

With this change, all node URLs should take us directly to their correct documentation pages, and we can further clean up some of the manual redirects in the docs.

@mieky mieky requested a review from krynble September 26, 2022 11:14
@linear
Copy link

linear bot commented Sep 26, 2022

N8N-4715

@mieky mieky marked this pull request as ready for review September 26, 2022 11:23
@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team node/improvement New feature or request labels Sep 26, 2022
Copy link
Contributor

@StarfallProjects StarfallProjects left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Copy link
Contributor

@ivov ivov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. URL is case-insensitive so the link will work, but should we be allowing for this casing divergence? n8n-nodes-base.activecampaigntrigger vs. n8n-nodes-base.activeCampaignTrigger The canonical name is camelCased after the prefix. No immediate risk that I can think of, but I would avoid this divergence if we can help it.
  2. With the help of a few custom scripts, I was able to locate and fix a couple of 404 links, malformed JSONs, and unnecessary redirects. Can you add a task to automate this in future? Via CI or an n8n workflow.
  3. npx prettier --write nodes/**/*.node.json Can you update the format command to format these files as well?

@StarfallProjects
Copy link
Contributor

  1. URL is case-insensitive so the link will work, but should we be allowing for this casing divergence? n8n-nodes-base.activecampaigntrigger vs. n8n-nodes-base.activeCampaignTrigger The canonical name is camelCased after the prefix. No immediate risk that I can think of, but I would avoid this divergence if we can help it.

Our hosting automatically lowercases URLs (this is the background to making the links lowercase)

@ivov
Copy link
Contributor

ivov commented Sep 26, 2022

  1. URL is case-insensitive so the link will work, but should we be allowing for this casing divergence? n8n-nodes-base.activecampaigntrigger vs. n8n-nodes-base.activeCampaignTrigger The canonical name is camelCased after the prefix. No immediate risk that I can think of, but I would avoid this divergence if we can help it.

Our hosting automatically lowercases URLs (this is the background to making the links lowercase)

That's unfortunate. Please disregard.

krynble
krynble previously approved these changes Sep 27, 2022
Copy link
Contributor

@krynble krynble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

Perhaps we can also change

return 'https://docs.n8n.io/nodes/' + (nodeType.documentationUrl || nodeType.name) + '?utm_source=n8n_app&utm_medium=node_settings_modal-credential_link&utm_campaign=' + nodeType.name;
so it also reflects the new URLs? Without this change n8n continues sending users to the wrong URL, requiring the 301 to be there forever.

@mieky
Copy link
Member Author

mieky commented Sep 28, 2022

Perhaps we can also change (link) so it also reflects the new URLs? Without this change n8n continues sending users to the wrong URL, requiring the 301 to be there forever.

Ahh, great catch! Thanks for pointing this out, I'll check this one.

@mieky mieky dismissed stale reviews from krynble and StarfallProjects via 63b8b32 September 28, 2022 11:05
Comment on lines +440 to 451
const { categories, subcategories, resources: allResources, alias } = require(`${filePath}on`); // .js to .json

const resources = pick(allResources, ['primaryDocumentation', 'credentialDocumentation']);

// eslint-disable-next-line @typescript-eslint/no-unsafe-return
return {
...(categories && { categories }),
...(subcategories && { subcategories }),
...(resources && { resources }),
...(alias && { alias }),
};
}
Copy link
Member Author

@mieky mieky Sep 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking is that in the long run the only maintainable way is to get the doc links directly from the node descriptor (codex). I now exposed resources.primaryDocumentation and resources.credentialDescription so that the UI can access and use those, instead of having to generate the URLs dynamically from parts.

'' : // Don't show documentation link for community nodes if the URL is not an absolute path
`https://docs.n8n.io/credentials/${type.documentationUrl}/?utm_source=n8n_app&utm_medium=left_nav_menu&utm_campaign=create_new_credentials_modal`;
`${BUILTIN_CREDENTIALS_DOCS_URL}${type.documentationUrl}/?utm_source=n8n_app&utm_medium=left_nav_menu&utm_campaign=create_new_credentials_modal`;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parametrized the credential link URL prefix.

Comment on lines +46 to +50
// Built-in node documentation available via its codex entry
const primaryDocUrl = nodeType.codex?.resources?.primaryDocumentation?.[0]?.url;
if (primaryDocUrl) {
return primaryDocUrl + utmTags;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All our built-in nodes (with the exception of the StickyNote, I think) have a primary documentation link defined in their descriptor, so let's use that directly where it applies.

Comment on lines +1628 to +1631
resources?: {
credentialDocumentation?: DocumentationLink[];
primaryDocumentation?: DocumentationLink[];
};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exposing documentation links as part of a node's CodexData.

@@ -18,7 +18,7 @@
"dev": "npm run watch",
"build": "tsc && gulp build:icons && gulp build:translations",
"build:translations": "gulp build:translations",
"format": "cd ../.. && node_modules/prettier/bin-prettier.js --write \"packages/nodes-base/**/*.ts\"",
"format": "cd ../.. && node_modules/prettier/bin-prettier.js --write \"packages/nodes-base/**/*.{ts,json}\"",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatting also JSON files now.

@mieky
Copy link
Member Author

mieky commented Sep 28, 2022

@krynble @ivov Thanks for the great input! I refactored how the documentation links are formed, and so far all the ones I clicked would seem to open the correct URL directly. It's possible there's a case I might have missed, but the logic overall seems quite solid. I also updated some more old links that obviously would either get straight up redirected or were just broken.

Also created a N8N-4791 for including the helper scripts in the repo, to make it easier to deal with these links in the future. Pointed my most notable changes as line comments.

@mieky mieky requested a review from krynble September 28, 2022 12:15
Copy link
Contributor

@krynble krynble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mieky mieky merged commit 6e8e4f5 into master Sep 29, 2022
@mieky mieky deleted the n8n-4715-update-node-doc-links branch September 29, 2022 10:33
@n8n-assistant n8n-assistant bot added the Upcoming Release Will be part of the upcoming release label Sep 29, 2022
@janober
Copy link
Member

janober commented Sep 30, 2022

Got released with [email protected]

@janober janober removed the Upcoming Release Will be part of the upcoming release label Sep 30, 2022
valya pushed a commit to valya/n8n that referenced this pull request Nov 8, 2022
…n-io#4191)

* fix(nodes-base): fix and harmonize all primaryDocumentation links

* feat(workflow, cli): expose documentation links to UI via node codex

* fix(editor-ui): link to correct node and credential documentation URLs

* config(nodes-base): update 'format' script to also format node descriptor json

* chore: fix outdated links to node reference documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
n8n team Authored by the n8n team node/improvement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants